Skip to content

Support V8 10.5 #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jun 30, 2022
Merged

Support V8 10.5 #490

merged 6 commits into from
Jun 30, 2022
+33 −3

Conversation

stesie
Copy link
Member

@stesie stesie commented Jun 24, 2022

V8 10.5 uses c++17 features (is_lvalue_reference_v), hence we need to adapt our config process to detect c++17 and use that (opposed to c++14 like we did before)

With that configuration works, compilation seems to pass pretty far but in the end fails with

/app/v8js_main.cc: In function 'int zm_shutdown_v8js(int, int)':
/app/v8js_main.cc:165:11: error: 'ShutdownPlatform' is not a member of 'v8::V8'
  165 |   v8::V8::ShutdownPlatform();
      |           ^~~~~~~~~~~~~~~~
make: *** [Makefile:218: v8js_main.lo] Error 1

Needs to be "investigated" where that function went to :)

refs #489

@stesie
Copy link
Member Author

stesie commented Jun 24, 2022 

/app/v8js_variables.cc: In function 'void v8js_register_accessors(std::vector<v8js_accessor_ctx*>*, v8::Local<v8::FunctionTemplate>, zval*, v8::Isolate*)':
/app/v8js_variables.cc:83:212: error: 'v8::AccessorSignature' has not been declared
   83 | ast<int>(ZSTR_LEN(property_name))), v8js_fetch_php_variable, NULL, v8::External::New(isolate, ctx), v8::PROHIBITS_OVERWRITING, v8::ReadOnly, v8::AccessorSignature::New(isolate, php_obj_t));
      |                                                                                                                                                  ^~~~~~~~~~~~~~~~~

make: *** [Makefile:230: v8js_variables.lo] Error 1

@stesie
Copy link
Member Author

stesie commented Jun 24, 2022

If you've compiled V8 in default configuration it'll have sandbox enabled. If so, you need to pass -DV8_ENABLE_SANDBOX to php-v8js as well. Like so:

./configure --with-v8js=/opt/libv8-10.5 LDFLAGS="-lstdc++" CPPFLAGS="-DV8_COMPRESS_POINTERS -DV8_ENABLE_SANDBOX"

@stesie
Copy link
Member Author

stesie commented Jun 24, 2022 •
edited
Loading

With these changes php-v8js compiles just fine with V8 10.5.81 and also passes the test suite. Please note that this PR is based on php7 branch, hence I've tried PHP 7.4.3

This PR needs some more love, especially

  • conditionally call either ShutdownPlatform or DisposePlatform, depending on V8 version
  • mention the -DV8_ENABLE_SANDBOX flag in README (and maybe simply ignore it on older V8 version so it's safe to unconditionally pass it)
  • auto-detect V8 sandbox and initialize it if needed

But feel free to already give it a try, give feedback and tell whether it works for you.

cc @temuri416

@stesie stesie marked this pull request as ready for review June 27, 2022 16:02
@stesie stesie mentioned this pull request Jun 27, 2022
Open
@stesie stesie changed the base branch from php8 to php7 June 30, 2022 13:19
@stesie stesie merged commit 9afd1a9 into phpv8:php7 Jun 30, 2022
@magdutra magdutra mentioned this pull request Jan 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@stesie